Until the previous functions and resource are deprecated and removed, users would import the reworked integration with the following:

from dagster_fivetran.v2 import DagsterFivetranTranslator, FivetranWorkspace

The new version of the integration won't actually be v2, so perhaps another import path would be better?

This is mainly to avoid name collisions between both previous and new translators. We could also name this translator differently if we really want users to import with from dagster_fivetran import FivetranWorkspace, but Dagster[IntegrationName]Translator is the naming convention for new integrations.

I think we should just call it something different than *.v2. Maybe experimental for a release -> 1.0 the integration, then we switch the old stuff to be under .legacy and make the new stuff exist under the top level package. @PedramNavid and @C00ldudeNoonan would probably have thoughts.

That makes sense - I updated the folder name for experimental in the previous PR in this stack.

I'm assuming this might be inherited from existing patterns, but I do find this kinda awkward. Creates a weird footgun where you can implement get_connector_spec in a way that does not utilize get_connector_asset_key, right?

I wonder if we should just have get_connector_spec and then force people to use get_connector_spec(...).asset_key.

To make this more efficient, we could cache the get_connector_spec result per datum.

I'm assuming this might be inherited from existing patterns, but I do find this kinda awkward.

Yes exactly. A similar discussion came up here.

I'm in favor of this change - I agree that this pattern is error prone.

It was kinda easier for a user to override the get_asset_key function, to add a prefix for instance. We could recommend doing something like the following instead, which looks a bit awkward but is less error prone.

class CustomDagsterTranslator(DagsterTranslator):
    def get_asset_spec(self, props) -> AssetSpec:
        asset_spec = super().get_asset_spec(props)
        return asset_spec._replace(asset_key=asset_spec.asset_key.with_prefix("my_prefix"))

@benpankow Any thoughts? I can update the other translators if we all agree on this.

I think we have better fxns for updating params on asset specs now too: replace_attributes top level fxn. But yea I think this pattern is good. Having an example people can copy paste, even if it looks slightly awkward, is much preferable to introducing a footgun.

Have we made the other integrations where this fxn exists public yet @maximearmstrong ? / would like to understand the status / experimentality there.

I'm on board with this change - I think our initial push for get_asset_key was to avoid doing extra work to construct an entire asset spec when we would discard most of it & just retrieve the key, but Chris made a good point that any asset whose key we're generating we'll also be generating a spec for, either before or after. As long as we can cache the spec generation, we don't end up doing extra work in that case.

That's fair - we can remove the get_asset_key method in favor of the replace_attributes fnx for customization.

Have we made the other integrations where this fxn exists public yet

The get_asset_spec method is used in the translator of each BI integrations, and these translators are not marked as experimental. I think that method is pretty stable at this point.

The get_asset_key method is also not marked as experimental for the BI integrations, so that implies a deprecation cycle, but the risk seems pretty low here.

Yea if we're going to do it, we should probably do so soon. Want to post a public discussion about it to make sure we get buy in from @schrockn @benpankow ; do we have anyone customizing BI translators to our knowledge yet?

To my knowledge, no one is customizing the Tableau and Power BI translators yet. @benpankow Do you know if anyone is customizing the Sigma and Looker ones?

I can post the public discussion with code snippets to get consensus on this.

For this PR, I will remove the get_asset_key method - we can easily add it depending on the outcome of the discussion.

yep that sounds good to me!

shouldn't we also have get_destination_asset_key?

We will leverage the previous translator instead. See updated PR description.

shouldn't we also have get_destination_spec?

I'm not sure this matches the way that we model Fivetran assets, at least in the current form - each connector can sync one or many tables, each of which we represent as an asset. Destinations may not make sense to model as an asset at all, since they just represent a storage destination but not a specific e.g. table

Re-reading the code after reading you comment, that makes a lot of sense. I will adjust the code.

I moved things around so that we leverage the translator as it was implemented, while saving the state with the raw API data. See updated PR description for the details.

Both FivetranContentType.CONNECTOR and FivetranContentType.DESTINATION need to be handled in the get_asset_key and get_asset_spec methods. Consider adding elif branches for FivetranContentType.DESTINATION that call corresponding get_destination_asset_key and get_destination_spec methods. These methods should be added as NotImplemented stubs, similar to the existing connector methods. The check.assert_never should only be hit for invalid enum values that aren't part of FivetranContentType.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Do we still want/need this class, FivetranContentType etc if it's not getting fed into the translator?

In particular I think having the content type isn't necessary, since we aren't exposing this data to the user directly

I think it's worth keeping FivetranContentType for a few reasons:

• The content data for Fivetran is a JSON object returned by the API for both connectors and destinations. Using FivetranContentType, it's easier to keep track of the type of the FivetranContentData.
  • This pattern is used for Power BI and Tableau, where the content data is also a JSON object returned by an API.
• The implementation of fetch_fivetran_workspace_data leverages the FivetranContentType enum to create the FivetranWorkspaceData.
  • We use FivetranContentType when creating FivetranContentData objects and the FivetranWorkspaceData object, to differentiate connectors and destinations. See usage in subsequent PR, in [4/n][dagster-fivetran] Implement fetch_fivetran_workspace_data #25788 and [5/n][dagster-fivetran] Implement FivetranWorkspaceData to FivetranConnectorTableProps method #25797

Overall, I'm in favor of keeping this to keep our design pattern for XWorkspaceData and fetch_x_workspace_data as consistent as possible across integrations.

I think we want to delete this now right?

Yes, done in 6eb1099